feat: Add redis caching, ttl, on initial endpoints#280
feat: Add redis caching, ttl, on initial endpoints#280zaydaanjahangir wants to merge 5 commits intomainfrom
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #280 +/- ##
========================================
+ Coverage 2.02% 9.53% +7.50%
========================================
Files 82 125 +43
Lines 3064 5455 +2391
Branches 24 0 -24
========================================
+ Hits 62 520 +458
- Misses 3002 4899 +1897
- Partials 0 36 +36
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
…GenerateNU/selfserve into feat/feat-redis-caching-phase-1
danctila
left a comment
danctila
left a comment
There was a problem hiding this comment.
just a couple cmmts
one thing i noticed was there isn't cache invalidation on writes. it looks like UpdateUser, UpdateProfilePicture, etc passthrough to the DB without touching the cache so a write followed by a read right after would return stale data until TTL dies. I think these need to delete or overwrite the cache key on sucess of. a write.
|
|
||
| type CachedGuestsRepository struct { | ||
| cache *JSONCache | ||
| next storage.GuestsRepository |
There was a problem hiding this comment.
all the other cached wrappers define a local interface for its next but this one imports directly from the pg package?
Yeah, I'm pretty sure I avoided invalidation on the first phase out of simplicity. Since we're out of featurethon I'll update this with a more fleshed out implementation, probably over a couple PRs. Thx for review 🐐 |
Dao-Ho
left a comment
Dao-Ho
left a comment
There was a problem hiding this comment.
Some more cmts on top of Dylan's
| require.NoError(t, err) | ||
| assert.Equal(t, "localhost:6379", cfg.Addr) | ||
| assert.Equal(t, "", cfg.Password) | ||
| } |
There was a problem hiding this comment.
How would this test look in a prod container? 🤔
| goredis "github.com/redis/go-redis/v9" | ||
| ) | ||
|
|
||
| var ErrCacheMiss = errors.New("cache miss") |
| return | ||
| } | ||
| c.logger.Warn("redis cache write failed", "key", key, "err", err) | ||
| } |
There was a problem hiding this comment.
This looks good but feels like this should be separated into two different layers.
One for the abstracted cache layer and the other for the Redis access itself.
Caller for our KV store shouldn't need to know how our Redis access works under the hood, also would just be good abstraction for future caching impl if we want to expand on it
4 participants
Cached endpoints: